Skip to content

Conversation

@sarnex
Copy link
Contributor

@sarnex sarnex commented Jun 9, 2025

Fix merge with my upstream change to refactor the device kernel attribute here.

@sarnex sarnex marked this pull request as ready for review June 9, 2025 19:52
@sarnex sarnex requested review from a team as code owners June 9, 2025 19:52
@sarnex
Copy link
Contributor Author

sarnex commented Jun 9, 2025

Hi @intel/dpcpp-cfe-reviewers @premanandrao @Fznamznon , can someone take a look at this? It's not complicated but it's not really trivial either. Thanks!

Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I'm slightly worried about this, because we use sycl_kernel attribute in the downstream and we managed to upstream it at some point, but the clang upstreaming team last year decided that we should invent a new attribute for these purposes called sycl_kernel_entry point and we will need to clean this mess at some point, but that is not your problem atm. cc @tahonermann

// so handle that case manually.
return A.getAttributeSpellingListIndex() == Keyword_kernel ||
A.getAttrName()->getName() == "kernel";
(A.getAttrName() && A.getAttrName()->getName() == "kernel");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this change specific to this downstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it's not but the issue was only exposed downstream, I'll fix it upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarnex
Copy link
Contributor Author

sarnex commented Jun 10, 2025

btw about sycl_kernel_entry_point, let me know if you need any help, ideally we don't need a new clang attribute if we're just specifying that it's a kernel, but if it represents more than that, then probably we do new a new attr.

@sarnex
Copy link
Contributor Author

sarnex commented Jun 10, 2025

thanks mariya!

@sarnex sarnex merged commit 7c024bc into intel:sycl-web Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants